fix(cli): baseline suppression in recursive multi-skill scans (#201) by wernerkasselman-au · Pull Request #205 · NVIDIA/SkillSpector

wernerkasselman-au · 2026-06-24T23:40:15Z

What this fixes

Closes #201. The suppression work (#88) and the multi-skill work (#136) did not
compose in scan --recursive. This PR carries two related fixes.

The suppression options were silently dropped. --baseline and
--show-suppressed were accepted on the command line and then not threaded
into the recursive path, so a baselined finding was not actually suppressed:
it still counted toward each sub-skill's risk score and could still flip the
exit code to 1. The single-skill path honoured both options, so only the
recursive path regressed, which is exactly the mode you reach for to scan a
whole skill collection in one pass.
The recursive report over-counted findings. Even with suppression
threaded through, the multi-skill summary table and the combined JSON report
counted filtered_findings, which the report node returns as the full
pre-partition set (kept plus baseline-suppressed); it never reduces that list
to active-only. So the count reported pre-suppression totals: a fully
baselined sub-skill showed score 0 but a non-zero finding_count,
disagreeing with its own report body and risk score.

The change

Threading (065d8fd). _scan_multi_skill() now accepts baseline and
show_suppressed, the recursive dispatch in scan() passes them, and they are
forwarded into the per-skill _scan_state(...) call, mirroring the single-skill
path.

Counting (b93da63). New _result_finding_count() helper computes the active
count as len(filtered_findings) - len(suppressed_findings) when
filtered_findings is a list (partition_findings guarantees
kept + suppressed == filtered_findings), and falls back to the raw findings
length only when filtered_findings is absent or not a list, without
subtracting there since raw findings are not the population that produced
suppressed_findings. It is used at both the summary table and the combined
JSON report.

Tests

test_cli_recursive_forwards_baseline_and_show_suppressed: builds a
two-sub-skill collection, generates a real baseline against one sub-skill,
then captures the state handed to the graph per sub-skill and asserts the
baseline was loaded and show_suppressed is set. On the old code the state
carried neither key.
test_cli_recursive_json_finding_count_excludes_suppressed: mirrors the real
report return shape (filtered_findings holds every finding,
suppressed_findings the suppressed subset) across a fully suppressed
sub-skill (active 0) and a partially suppressed one (active 1). On the old
code both counted as 3.
test_result_finding_count_branches: unit-covers every branch of the helper
(normal, no-baseline, raw fallback, non-list, empty, malformed clamp).

uv run ruff check src/ tests/           # clean
uv run ruff format --check src/ tests/  # clean
uv run pytest -q                        # 1258 passed

Refs #201

…port The recursive summary table and combined JSON report computed finding_count as `filtered_findings or findings`, which falls through to the unfiltered findings when suppression empties filtered_findings, since an empty list is falsy. A fully baselined sub-skill then showed score 0 but a non-zero finding_count. Add _result_finding_count(), which falls back to raw findings only when filtered_findings is absent (the report node always sets it, possibly empty after suppression), and use it at both the summary table and the combined JSON report. Add a regression test asserting the combined JSON finding_count is 0 for a fully suppressed sub-skill. Surfaced by an independent review of NVIDIA#205. Refs NVIDIA#201 Signed-off-by: Werner Kasselman <[email protected]>

rng1995

Approving — fixes #201 by threading --baseline/--show-suppressed into the recursive multi-skill path (they were silently dropped). Also a real correctness fix: _result_finding_count() replaces filtered_findings or findings, which mis-counted a fully-suppressed skill (empty list is falsy, so the or fell through to the unfiltered findings). Both behaviors are covered by new tests.

Non-blocking: this and #209 both edit _scan_multi_skill — whichever merges second will need a small rebase.

rng1995 · 2026-07-10T20:18:08Z

Please resolve merge conflicts @wernerkasselman-au

…cans _scan_multi_skill did not accept or forward the suppression options, and the recursive dispatch in scan() did not pass them, so scan --recursive silently ignored --baseline and --show-suppressed. Suppressed findings still counted toward each sub-skill's score and could flip the exit code to 1, breaking the incremental baseline workflow precisely in the mode meant for skill collections. Add baseline and show_suppressed parameters to _scan_multi_skill, pass them from the recursive dispatch, and forward them into the per-skill _scan_state call, mirroring the single-skill path. Add a regression test that captures the per sub-skill graph state and asserts the baseline and show_suppressed are present. Refs NVIDIA#201 Signed-off-by: Werner Kasselman <[email protected]>

…port The recursive summary table and combined JSON report computed finding_count as `filtered_findings or findings`, which falls through to the unfiltered findings when suppression empties filtered_findings, since an empty list is falsy. A fully baselined sub-skill then showed score 0 but a non-zero finding_count. Add _result_finding_count(), which falls back to raw findings only when filtered_findings is absent (the report node always sets it, possibly empty after suppression), and use it at both the summary table and the combined JSON report. Add a regression test asserting the combined JSON finding_count is 0 for a fully suppressed sub-skill. Surfaced by an independent review of NVIDIA#205. Refs NVIDIA#201 Signed-off-by: Werner Kasselman <[email protected]>

wernerkasselman-au · 2026-07-10T21:37:50Z

Rebased onto current main and resolved the merge conflict in tests/unit/test_cli.py. The conflict was purely additive: main added test_scan_multi_skill_markdown_output_to_file / test_scan_multi_skill_json_output_unchanged, and this branch added test_cli_recursive_forwards_baseline_and_show_suppressed; all three are kept. The cli.py change (threading baseline / show_suppressed through _scan_multi_skill and _scan_state) merged cleanly and matches _scan_state's current signature on main.

Local verification: tests/unit/test_cli.py 11 passed, ruff format --check and ruff check both clean. @rng1995

wernerkasselman-au · 2026-07-10T21:59:13Z

Hi @rng1995,

Both are sorted on my side.

For #205, I have rebased onto current main and resolved the one conflict in tests/unit/test_cli.py. It was purely additive (main had added test_scan_multi_skill_markdown_output_to_file and test_scan_multi_skill_json_output_unchanged, and this branch added test_cli_recursive_forwards_baseline_and_show_suppressed), so I kept all three rather than dropping either side. Two commits sit on the branch: the first threads baseline and show_suppressed through _scan_multi_skill and _scan_state (that merged cleanly and lines up with _scan_state's current signature on main), and the second is a small follow-on fix so the recursive multi-skill report counts post-suppression findings, via a _result_finding_count helper, so a fully suppressed sub-skill now reports 0 rather than its raw pre-baseline count (covered by test_cli_recursive_json_finding_count_excludes_suppressed). Locally the CLI suite is green (11 passed) and both ruff format --check and ruff check are clean. It reads as MERGEABLE now, though the CI run is sitting at action_required, so it will need one of you to approve the fork workflow before the checks go green (nothing I can trigger from my side).

For #207, I have closed it as obsolete. The ruff 0.15.2 formatting it applied is already on main, ruff format --check src/ tests/ passes there across all 144 files, and the branch was based on an older main (7bc9c0f) whose head predates newer commits to static_runner.py and test_llm_analyzer_base.py (69ac3ba, 1c796a5), so merging it as-is would actually have reverted those. Rebasing onto main drops its only commit as already upstream, which leaves nothing to merge. Happy to reopen and reformat against current main if you would rather keep a dedicated formatting PR, just let me know.

Thanks,
Werner

wernerkasselman-au · 2026-07-10T21:59:52Z

@rng1995 when you have a moment, could you approve the workflow run on this PR? It is a fork PR, so the CI is sitting at action_required and needs a maintainer to kick it off before the checks can go green. Everything is green locally on my side (11 CLI tests passing, ruff clean), just need the gate opened. Thanks.

rng1995

[Automated SkillSpector Review]

Summary: Fixes #201 — in scan --recursive, --baseline and --show-suppressed were accepted but silently dropped. The PR threads both options through _scan_multi_skill() into the per-skill _scan_state() call, and adds _result_finding_count() to fix the falsy empty-list fallback (filtered_findings or findings) that over-reported the finding count for fully suppressed sub-skills. Both fixes are correct and covered by meaningful regression tests (test_cli_recursive_forwards_baseline_and_show_suppressed, test_cli_recursive_json_finding_count_excludes_suppressed); I verified all tests in the touched files pass against this head (23/23). Combined JSON schema is preserved. (This review supersedes the earlier automated approval, which predates the rebase to the current head.)

Blocker

Unhandled exception + wrong exit code for a bad --baseline path in recursive mode. _scan_multi_skill() is dispatched before the try/except in scan(), and _scan_state() (which calls load_baseline()) runs outside the per-skill try block. Verified empirically: scan <collection> -r --baseline /nonexistent.yaml prints a raw traceback and exits 1 — the "risk found" code — while the single-skill path prints Error: Baseline file not found: ... and exits 2. For a CI-gating security tool this conflates an operator typo with a security finding, and it contradicts the comment in _scan_state ("mapped to exit code 2 by scan()"). Suggested fix: load the baseline once in scan() inside the existing try (passing the loaded Baseline down), or wrap the recursive dispatch in equivalent FileNotFoundError/ValueError handling. Loading once also fixes the related inefficiency below.

Non-blocking

load_baseline(baseline) is re-read and re-parsed once per sub-skill (N times for N skills). Load once and pass the parsed Baseline object.
Cross-sub-skill suppression semantics are worth documenting: fingerprints hash rule_id|file|lines|message with file relative to each sub-skill root, so a fingerprint generated against one sub-skill will suppress an identical finding at the same relative path in a sibling sub-skill; conversely, path-scoped rules cannot target a specific sub-skill in recursive mode (the sub-skill prefix is not in finding.file). Consistent with single-skill semantics, but a maintainer may want a doc note or a follow-up to prefix relative_path.
The same falsy-fallback pattern the PR fixes still exists in the baseline command (cli.py ~line 542) and mcp_server.py:108; _result_finding_count() could be reused there in a follow-up.

The core change is good and well-tested — once the baseline error path is handled to match the single-skill contract, this is ready.

rng1995 · 2026-07-14T10:06:51Z

+                no_llm,
+                yara_rules_dir,
+                verbose,
+                baseline=baseline,


Blocker: this dispatch happens before the try/except in scan(), and load_baseline() inside _scan_state() can raise FileNotFoundError/ValueError. Verified: scan <dir> -r --baseline /nonexistent.yaml emits a raw traceback and exits 1 (the "risk found" code), while the single-skill path prints a clean error and exits 2. Please load the baseline once here inside the existing try (or wrap this call in equivalent handling) so recursive mode keeps the documented exit-code contract.

rng1995 · 2026-07-14T10:06:51Z

+            format,
+            no_llm,
+            yara_rules_dir=yara_dir,
+            baseline=baseline,


load_baseline() runs here once per sub-skill, re-reading and re-parsing the same file N times. Load it once in scan() (which also fixes the unhandled-exception blocker) and pass the parsed Baseline object down.

rng1995 · 2026-07-14T10:06:51Z

    }


+def _result_finding_count(result: dict[str, object]) -> int:


Good fix for the falsy empty-list fallback. Note the same filtered_findings or findings pattern remains in the baseline command (~line 542) and mcp_server.py:108 — consider reusing this helper there in a follow-up.

…port The recursive summary table and combined JSON report computed finding_count from filtered_findings, but the report node returns filtered_findings as the full pre-partition set (kept plus baseline-suppressed) and lists the suppressed subset separately under suppressed_findings; it never reduces filtered_findings to active-only. Counting len(filtered_findings) therefore reported pre-suppression totals, so a baselined sub-skill showed score 0 but a non-zero finding_count, disagreeing with the per-skill report body and risk score. Add _result_finding_count(), which returns len(filtered_findings) - len(suppressed_findings) when filtered_findings is a list (partition_findings guarantees kept + suppressed == filtered_findings, so this is the active count), and falls back to the raw findings length only when filtered_findings is absent or not a list, without subtracting there since raw findings are not the population that produced suppressed_findings. Use it at both the summary table and the combined JSON report. Add a regression test covering a fully suppressed sub-skill (active 0) and a partially suppressed one (active 1), plus a unit test exercising each branch of the helper. Refs NVIDIA#201 Signed-off-by: Werner Kasselman <[email protected]>

wernerkasselman-au mentioned this pull request Jun 25, 2026

style: apply ruff 0.15.2 formatting to drifted files (unblocks CI format check) #207

Closed

rng1995 approved these changes Jun 27, 2026

View reviewed changes

rng1995 mentioned this pull request Jun 27, 2026

fix(cli): write concatenated multi-skill report to --output for non-JSON formats #209

Merged

4 tasks

wernerkasselman-au force-pushed the fix/recursive-baseline-suppression branch from b45b4ee to ccfbdf4 Compare July 10, 2026 21:29

rng1995 requested changes Jul 14, 2026

View reviewed changes

wernerkasselman-au force-pushed the fix/recursive-baseline-suppression branch from ccfbdf4 to b93da63 Compare July 15, 2026 01:49

wernerkasselman-au changed the title ~~fix(cli): thread --baseline and --show-suppressed through recursive scans (#201)~~ fix(cli): baseline suppression in recursive multi-skill scans (#201) Jul 15, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(cli): baseline suppression in recursive multi-skill scans (#201)#205

fix(cli): baseline suppression in recursive multi-skill scans (#201)#205
wernerkasselman-au wants to merge 2 commits into
NVIDIA:mainfrom
wernerkasselman-au:fix/recursive-baseline-suppression

wernerkasselman-au commented Jun 24, 2026 •

edited

Loading

Uh oh!

rng1995 left a comment

Uh oh!

rng1995 commented Jul 10, 2026

Uh oh!

wernerkasselman-au commented Jul 10, 2026

Uh oh!

wernerkasselman-au commented Jul 10, 2026

Uh oh!

wernerkasselman-au commented Jul 10, 2026

Uh oh!

rng1995 left a comment

Uh oh!

rng1995 Jul 14, 2026

Uh oh!

rng1995 Jul 14, 2026

Uh oh!

rng1995 Jul 14, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		}


		def _result_finding_count(result: dict[str, object]) -> int:

Uh oh!

Conversation

wernerkasselman-au commented Jun 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What this fixes

The change

Tests

Uh oh!

rng1995 left a comment

Choose a reason for hiding this comment

Uh oh!

rng1995 commented Jul 10, 2026

Uh oh!

wernerkasselman-au commented Jul 10, 2026

Uh oh!

wernerkasselman-au commented Jul 10, 2026

Uh oh!

wernerkasselman-au commented Jul 10, 2026

Uh oh!

rng1995 left a comment

Choose a reason for hiding this comment

Blocker

Non-blocking

Uh oh!

rng1995 Jul 14, 2026

Choose a reason for hiding this comment

Uh oh!

rng1995 Jul 14, 2026

Choose a reason for hiding this comment

Uh oh!

rng1995 Jul 14, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

wernerkasselman-au commented Jun 24, 2026 •

edited

Loading